Code Review — window projection mode (SCALAR/AGG keywords)

Reviewed by GitHub Copilot (tsdb-code-review skill v0.1.0)

Summary

This PR introduces window projection mode (v3.4.2.0): window queries may now include plain column expressions in the SELECT list, causing each window to output all original rows instead of one aggregated row. Two new SQL keywords SCALAR/AGG disambiguate mode when only pseudocolumns/tags/constants are selected.

5 verified issues found (2 BLOCKER, 3 MAJOR):

🔴 BLOCKER

1. O(N²) complexity — checkWindowProjectionMode called per node in tree walk
source/libs/parser/src/parTranslater.c line 5413

checkWindowProjectionMode is called inside doCheckExprForGroupBy (a nodesRewriteExprs tree-walk callback, invoked O(T) times), and itself runs nodesWalkExprs(pSelect->pProjectionList, …) — another O(T) walk. For a SELECT tree of total size T, the group-by semantic check phase now costs O(T²).

Fix: Compute isScalarMode once before the walk and pass it via STranslateContext, or cache it in SSelectStmt. Do not compute it inside a per-node callback.

2. SCALAR/AGG accepted silently on queries without a window clause
source/libs/parser/src/parAstCreater.c line 2770

There is no validation that windowMode != WINDOW_MODE_NONE requires a window clause to be present. SELECT SCALAR c1 FROM t1 WHERE ts > 0 parses and translates without error. If doCheckExprForGroupBy is subsequently invoked for that query (e.g., triggered by ORDER BY context), checkWindowProjectionMode returns true (WINDOW_MODE_SCALAR, no agg/groupby/indefrows, ctx.hasScalarExpr = true), causing all partition-key and tag-column rewriting to be silently skipped and producing an incorrect query plan.

Fix:

// In translateSelectFrom or a dedicated checkWindowMode():
if (pSelect->windowMode != WINDOW_MODE_NONE && pSelect->pWindow == NULL) {
    return generateSyntaxErrMsg(&pCxt->msgBuf, TSDB_CODE_PAR_INVALID_OPTR_USAGE,
        "SCALAR/AGG can only be used with a window clause");
}

Also add || pSelect->pWindow == NULL to the early-return guard in checkWindowProjectionMode.

🟠 MAJOR

3. Potential memory leak of pProjExprInfo in initIndefRowsRuntime error path
source/libs/executor/src/executorInt.c line 1337–1340

createExprInfo allocates pProjExprInfo. If initExprSupp fails before it stores the pointer in pRuntime->projSupp.pExprInfo, the _return path's cleanupExprSupp(&pRuntime->projSupp) will not free it (because projSupp.pExprInfo is still NULL).

Fix: Transfer ownership immediately after allocation so cleanupExprSupp always handles it:

code = createExprInfo(pProjs, NULL, &pProjExprInfo, &numProjs);
QUERY_CHECK_CODE(code, lino, _return);
pRuntime->projSupp.pExprInfo = pProjExprInfo;  // transfer before initExprSupp
pProjExprInfo = NULL;
code = initExprSupp(&pRuntime->projSupp, pRuntime->projSupp.pExprInfo, numProjs, pFuncStore);
QUERY_CHECK_CODE(code, lino, _return);

Or verify that initExprSupp always stores the pointer before any failure point.

4. INTERVAL projection mode routes to stbSplSplitSessionOrStateForBatch — confirm correctness
source/libs/planner/src/planSpliter.c line ~935

In stbSplSplitWindowForCrossTable, INTERVAL windows in projection mode (!pWin->pFuncs && pWin->pProjs) are dispatched to stbSplSplitSessionOrStateForBatch, a function written for SESSION/STATE windows. These have different partitioning semantics from INTERVAL (no time-alignment, no cross-vnode merge). Using it for multi-vnode INTERVAL projection queries may produce incorrect data distribution or output ordering.

Fix: Add a multi-vnode integration test: SELECT _wstart, ts, c1 FROM stable PARTITION BY tbname INTERVAL(10s). If results are correct, add a comment explaining why this split strategy is intentionally reused here. If not, introduce a dedicated split path.

5. hasScalarExpr and checkWindowProjectionMode can diverge

There are two independent mode-detection mechanisms:

• checkWindowProjectionMode re-walks pProjectionList looking for COLUMN_TYPE_COLUMN nodes
• pSelect->hasScalarExpr is set in doCheckExprForGroupBy under !hasSelectFunc && !isRelatedToOtherExpr

These conditions are not equivalent and can diverge (e.g., a column that isRelatedToOtherExpr would be seen by checkWindowProjectionMode but not set hasScalarExpr). Downstream code in planLogicCreater.c gates pWindow->indefRowsFunc and projectionMode on hasScalarExpr, while the translator skip logic uses checkWindowProjectionMode. A mismatch between the two means the planner and the translator disagree on the query's mode.

Fix: Use a single cached field (e.g. bool windowScalarMode in SSelectStmt) as the sole source of truth, set once early in translateSelectFrom, and used by both the translator and the planner.

🟡 MINOR

• line vs lino naming — fillIndefRowsWindowPseudoCols uses line instead of lino (codebase convention, executorInt.c line 1297)
• Removed negative test without comment — parSelectTest.cpp line 337 drops run("SELECT c1 FROM t1 INTERVAL(10s)", TSDB_CODE_PAR_INVALID_OPTR_USAGE) without explanation; add a comment noting it is now valid in projection mode
• Missing comment on FILL restriction scope — the FILL mode restriction check only handles INTERVAL_WINDOW; other window types silently pass without a note explaining that they cannot carry a FILL node syntactically